Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Image attachment re-renders on message update #14207

Merged
merged 2 commits into from
Apr 25, 2019

Conversation

Kailash0311
Copy link
Contributor

Closes #5028 and #14092

Issue and research: On researching and playing around with codebase a bit I found out that message is re-created everytime someone updates the message, (e.g: Starring or adding a reaction as mentioned in the issues that this PR solves.)Since it is meteor's default reactivity system because of which it is happening, my resolution is a simple work-around.

Resolution: Add a onDestroy function to the lazyloadtemplate(This template is responsible for showing the preview of the attachment.) This template gets destroyed and recreated whenever there is some update in the message (e.g: Starring or adding a reaction). Workaround is a simple Template.onDestroy call that stores the value of loaded state before being destroyed because of the update in the message. So whenever the template is recreated, now this.loaded has the same state as what was before updating a message(or being destroyed).

@geekgonecrazy
Copy link
Contributor

@RocketChat/core if this solves... this would be amazing to include. An extremely annoying issue

@geekgonecrazy geekgonecrazy changed the title [FIX]lazyloadImage: Add onDestroy function. [FIX] attachment re-render on message update Apr 21, 2019
@geekgonecrazy geekgonecrazy changed the title [FIX] attachment re-render on message update [FIX] image attachment re-render on message update Apr 21, 2019
@geekgonecrazy
Copy link
Contributor

Does this work for others? Like say youtube embed?

@Kailash0311
Copy link
Contributor Author

Does this work for others? Like say youtube embed?

No, it doesn't because youtube embed doesn't depend upon lazyloadImage package. But I guess the strategy would be the same there too.

@engelgabriel engelgabriel added this to the 1.0.0 milestone Apr 22, 2019
@rodrigok
Copy link
Member

@Kailash0311 Ok, but with your solution ALL images will share the same control variable right? Doesn't that defeats the purpose?

@rodrigok
Copy link
Member

A possible solution may be creating a Set to act as a cache where you store the src and every time you create the template instance you define the loaded initial state as true if the src already exists in Set.

@rodrigok
Copy link
Member

To be clear, this will not stop the re render @geekgonecrazy , will just prevent the render of the preview/base64 image and then the original image, so it may blink less, but videos would back to the initial state due to the re render. To solve that we need to remove NRR or move to another template language,

@geekgonecrazy
Copy link
Contributor

Ah! So this is only preventing the blury base64 to regular. Since the other image is already downloaded it re-renders fast enough you don't notice it.

Drats.. I was hoping solving it really was this simple as its an extremely annoying issue.

Avoiding the blurry to clear flicker still would be a welcome change though

@Kailash0311
Copy link
Contributor Author

@Kailash0311 Ok, but with your solution ALL images will share the same control variable right? Doesn't that defeats the purpose?

Oh, yes. How could I not see that! That is why new images were never loading the preview image.
Thanks!

@Kailash0311
Copy link
Contributor Author

Kailash0311 commented Apr 24, 2019

A possible solution may be creating a Set to act as a cache where you store the src and every time you create the template instance you define the loaded initial state as true if the src already exists in Set.

@rodrigok Do you mean using set method in Session package that meteor provides? Or a normal array of objects?

@Kailash0311
Copy link
Contributor Author

Kailash0311 commented Apr 24, 2019

To be clear, this will not stop the re render @geekgonecrazy , will just prevent the render of the preview/base64 image and then the original image, so it may blink less, but videos would back to the initial state due to the re render. To solve that we need to remove NRR or move to another template language,

Yes, this will not stop re-render as the message itself is being re-rendered altogether. This will just skip the loading of the preview image and directly load the main image.

@rodrigok
Copy link
Member

@Kailash0311 I mean the Set class https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

Copy link
Member

@rodrigok rodrigok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this changes ALL images will share the same control variable.

@Kailash0311
Copy link
Contributor Author

With this changes ALL images will share the same control variable.

Yes, I understood it.

@Kailash0311
Copy link
Contributor Author

Kailash0311 commented Apr 25, 2019

@rodrigok In the recent changes, the PREVIEW image will not be re-loaded when the message is re-rendered.

@rodrigok rodrigok changed the title [FIX] image attachment re-render on message update [FIX] Image attachment re-renders on message update Apr 25, 2019
@rodrigok rodrigok merged commit 12741f2 into RocketChat:develop Apr 25, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing reaction emoji to an image causes it to go back to 'click to load'.
4 participants